Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not fail if a namespace cannot be loaded. #4

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

jimhester
Copy link
Member

This fixes devtools failures when using sessioninfo

https://travis-ci.org/hadley/devtools/jobs/260288634#L823-L827

@gaborcsardi
Copy link
Member

Thanks! Wouldn't it be better no to try to load it at all?

The point of using getNamespaceVersion was to detect the rare case when the loaded version is different from the on-disk version. If the package is not loaded, we can just keep the loaded version empty.

Anyway, I can merge this now, and change it later.

@jimhester
Copy link
Member Author

Maybe, I can do that instead, one sec, will update the PR.

@codecov-io
Copy link

codecov-io commented Aug 2, 2017

Codecov Report

Merging #4 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #4   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         173    174    +1     
=====================================
+ Hits          173    174    +1
Impacted Files Coverage Δ
R/utils.R 100% <100%> (ø) ⬆️
R/loaded-packages.R 100% <100%> (ø) ⬆️
R/dependent-packages.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de1163...3806e7c. Read the comment docs.

@jimhester jimhester force-pushed the handle_namespace_failures branch from 96332d3 to c15817f Compare August 2, 2017 19:23
@jimhester
Copy link
Member Author

Ok updated to only get namespace versions for loaded namespaces.

@gaborcsardi
Copy link
Member

Are the NAs printed? How does the printed output look?

@gaborcsardi
Copy link
Member

I'll update the mocked test, don't worry about that.

@jimhester
Copy link
Member Author

Hmm NA actually causes errors from base::package_version() because it is not a valid package version, as done "". "0.0.0" works, but the output is kind of gross.

─ Packages ───────────────────────────────────────────────
 package           * version    date       source

 acepack             0.0.0 (!)  2016-10-29 CRAN (R 3.4.0)

 AnnotationDbi       0.0.0 (!)  2017-04-25 Bioconductor

 ape                 0.0.0 (!)  2017-02-14 CRAN (R 3.4.0)

 archive             0.0.0 (!)  2017-07-28 Github (jimhester/archive@a203312)

 assertive.base      0.0.0 (!)  2016-12-30 CRAN (R 3.4.0)

 assertthat          0.2.0      2017-04-11 CRAN (R 3.4.0)

@gaborcsardi
Copy link
Member

Yeah, I am afraid we need to change the print method.

@gaborcsardi
Copy link
Member

Somewhere around here:

badloaded <- package_version(x$loadedversion) !=

If it is not loaded, just use the on-disk version.

@jimhester jimhester force-pushed the handle_namespace_failures branch from c15817f to 3806e7c Compare August 2, 2017 19:52
@jimhester
Copy link
Member Author

Ok turns out if you use package_version(NA, strict = FALSE) we get an NA package version, which we can then handle in the print method, which looks nicer and should still catch the case where a loaded version is different than the installed version as before (3806e7c).

@gaborcsardi
Copy link
Member

Great! Thanks!

@gaborcsardi gaborcsardi merged commit be05f95 into r-lib:master Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants